-
Notifications
You must be signed in to change notification settings - Fork 591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
compute: VM.waitFor() #2047
compute: VM.waitFor() #2047
Conversation
Hey @NickZ, just wanted to let you know I appreciate this PR very much, and your level of thought into it. We've been on a bit of a release-after-release streak lately, so it's been hard to prioritize the review for this, but I will get to it, and your help has not been taken for granted. Thanks for your patience! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for your PR, and I am sorry it took us awhile to review -- that is on me.
I have offered a few comments, but this is generally in good shape. Let me know if you have any objections to the feedback offered.
packages/compute/src/vm.js
Outdated
/** | ||
* Interval for polling during waitFor. | ||
*/ | ||
var WAIT_FOR_POLLING_INTERVAL = 2000; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
packages/compute/src/vm.js
Outdated
* - "SUSPENDED" | ||
* - "TERMINATED" | ||
* @param {object=} options - Configuration object. | ||
* @param {number} options.timeout - The number of seconds to wait until |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
packages/compute/src/vm.js
Outdated
else if (options.timeout > 600) { | ||
options.timeout = 600; | ||
} | ||
// If timeout is less than 0, set to 0 seconds. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
packages/compute/src/vm.js
Outdated
callback: callback | ||
}); | ||
|
||
if (this.hasActiveWaiters === false) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
* var metadata = data[0]; | ||
* }); | ||
*/ | ||
VM.prototype.waitFor = function(status, options, callback) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@lukesneeringer what do you think about the error messages? Do you think they look OK? |
Am I blind? I do not see error messages. :boggle: |
packages/compute/src/vm.js
Outdated
self.waiters[index].timeout) { | ||
self.waiters[index].callback({ | ||
name: 'WaitForTimeout' | ||
},null); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
packages/compute/src/vm.js
Outdated
if (VALID_STATUSES.indexOf(status) === -1) { | ||
callback({ | ||
name: 'StatusNotValid' | ||
}, null); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@lukesneeringer Apologies, I added comments to point out where they were. |
Ah, okay. I was looking for sentences when I was scanning. I am fine with the codes, but send an |
@stephenplusplus This looks good to me. Do you see anything? |
Tests! I'll tag in and get on that. Thanks @NickZ! |
Previously, this would set the timeout to the default of 300 if the timeout was set to 0. Fixed so that it only checks to see if timeout is not set. Added JSdoc comments clarifying that if the options.timeout is set to 0, it will poll once and then timeout if the state is not reached.
WAIT_FOR_POLLING_INTERVAL and VALD_STATUSES should be set to private.
An invalid status now throws an Error, since this is a problem with how the function is called (like detatchDisk() does) A time out will return an Error in the callback , with the appropriate message, code, and name.
There is no apiResponse passed to the callback.
While iterating over all the waiters, if we happen to remove a waiter, it would skip the next item on the next iteration because the indexes changed. So, I changed it to wait until after it finishes iterating over all the waiters in the array to remove them. I also changed it to use a 'for of' loop since there was no longer a reason to keep the index around.
6aa4859
to
1d81e0d
Compare
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
@NickZ please take a look at my latest commit to see that all functionality is as intended. |
Looks good to me. I am ok with these changes. |
Awesome, thanks for the idea and all of the work on this! |
See issue #2015
This pull request implements
VM.waitFor()
. This function waits for the VM to be in a specified state, then calls the callback (or resolves the promise).example:
It performs this by calling
getMetadata()
in the background at a specified interval (currently2000
milliseconds).It takes in an
options
object, with a parametertimeout
. This is by default set to 300, but it can be set to a maximum of600
or a minimum of0
.Regarding Events
It was requested in the issue that I implement this using events. After some thought, I've decided against using events for this. I think in this case they offer too many opportunities for unexpected behavior and/or hammering the API.
Events work on things like Operations because the event flow goes like this:
Once the Operation is completed (or returned with an error), it will stay like that, forever. If a user calls
operation.on("complete")
, there's no problem with having to keep polling for statuses after it's reached "complete" or "error", because it will stay there.However, with a VM, it can go from RUNNING to SUSPENDED to STOPPED and back any number of times. The state is transient, and isn't as simple to handle as an Operation.
Say a user calls
vm.once("RUNNING")
. This is fine because we will wait until it enters the state "RUNNING", and the event triggers, and stops polling because there are no more listeners.However, say that a user calls
vm.on("RUNNING")
. It would be a reasonable assumption for the user to expect this to trigger every time the VM enters the "RUNNING" state, since they know that the VM state is transient. Because of this, this will require us to keep polling for status, forever, until the user removes the listener. This means a lot of superfluous API requests, and angry Google engineers.We could just treat every "on" as "once", and explicitly say so in the API documentation. However, I don't know of many things that do this, and it would be unexpected by the user who is used to "on" meaning "on", unless they carefully read the documentation.
We could keep the number of api requests to a minimum to not hammer the server, say, every 15 seconds, but it's possible for the VM to transition states and back in that amount of time, and the event wouldn't get triggered.
We could carry around every single operation that is performed on the VM and emit when those have emitted, but that would require a large amount of architecting, and we want this to work independent of any operation executed in the script (say, a user stops and starts the VM in the web console).
This is not to say that events should not be implemented, I just couldn't find satisfactory solutions for these issues, and that's why I decided not to implement them. If you come up with some solutions for these issues, let me know and I can take another crack at implementing this with events.
Design Considerations.
So anyway, this is what I kept in mind when I designed
VM.waitFor()
.I didn't want to passively enable people to ping the API continously, so I added an
options
field with atimeout
parameter. By default, this is 300 seconds, but the maximum is 600 seconds.Since it's a one-time event, it's predictable: it will callback when it reaches the specified state with the metadata, or it will return an error. It's independent of any operation, and will work whether or not the user had performed any operations on the VM in the script.
Issues
I'm not quite sure that I have error handling correct. If it times out, it passes back an error with a
name
field set to "WaitForTimeout". I'm not sure if this is the correct procedure, so if you could please take a look.